wallet: SQL-only signer account-secret fallback#1254
Open
yyforyongyu wants to merge 3 commits into
Open
Conversation
Coverage Report for CI Build 27161315132Warning No base build found for commit Coverage: 54.492%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
yyforyongyu
commented
Jun 3, 2026
yyforyongyu
left a comment
yyforyongyu
left a comment
Collaborator
Author
There was a problem hiding this comment.
LGTM (deepseek-v4-pro)
Collaborator
Author
|
LGTM 🔑 |
There was a problem hiding this comment.
Pull request overview
This PR enables wallet signing operations to derive private keys for SQL-only accounts (accounts present in the SQL store but not mirrored in legacy waddrmgr) by introducing an account-secret lookup contract on db.AccountStore and wiring a keyVault-based fallback into the signer.
Changes:
- Add
AccountStore.GetAccountSecret(+ pg/sqlite implementations + sqlc queries) to fetch encrypted account-level private key material (or detect watch-only / not-found). - Update
wallet/signerkey-resolution to fall back from legacywaddrmgrderivation to store-backed account secrets for SQL-only accounts. - Add unit/integration tests covering SQL-only signing paths and store contract behavior.
Reviewed changes
Copilot reviewed 13 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| wallet/signer.go | Adds store-backed key derivation fallback for SQL-only accounts and threads context.Context through signer key-resolution. |
| wallet/signer_test.go | Adds coverage for SQL-only account signing (unlocking script, DerivePrivKey, SignDigest, ECDH, ComputeRawSig) and error cases. |
| wallet/cache.go | Extends runtime cache seam to expose GetAccountSecret (currently pass-through). |
| wallet/internal/db/interface.go | Extends AccountStore interface with GetAccountSecret contract. |
| wallet/internal/db/data_types.go | Introduces AccountSecret + GetAccountSecretQuery data types. |
| wallet/internal/db/accounts_common.go | Adds GetAccountSecretQuery.Validate and backend-independent error for unsupported secrets. |
| wallet/internal/db/sqlite/accounts.go | Implements GetAccountSecret for SQLite store + row mapping and error mapping. |
| wallet/internal/db/pg/accounts.go | Implements GetAccountSecret for PostgreSQL store + row mapping and error mapping. |
| wallet/internal/db/kvdb/accountstore.go | Implements GetAccountSecret as unsupported (ErrAccountSecretUnavailable) for kvdb backend. |
| wallet/internal/db/itest/account_store_test.go | Adds itest verifying GetAccountSecret distinguishes spendable/watch-only/not-found outcomes. |
| wallet/internal/bwtest/mock/store.go | Updates store mock to satisfy the extended AccountStore interface. |
| wallet/internal/sql/sqlite/queries/accounts.sql | Adds GetAccountSecret SQL query for sqlite sqlc generation. |
| wallet/internal/sql/sqlite/sqlc/accounts.sql.go | Generated sqlite sqlc code for GetAccountSecret. |
| wallet/internal/sql/sqlite/sqlc/querier.go | Adds GetAccountSecret to sqlite sqlc Querier interface. |
| wallet/internal/sql/sqlite/sqlc/db.go | Prepares/closes the sqlite GetAccountSecret statement. |
| wallet/internal/sql/pg/queries/accounts.sql | Adds GetAccountSecret SQL query for pg sqlc generation. |
| wallet/internal/sql/pg/sqlc/accounts.sql.go | Generated pg sqlc code for GetAccountSecret. |
| wallet/internal/sql/pg/sqlc/querier.go | Adds GetAccountSecret to pg sqlc Querier interface. |
| wallet/internal/sql/pg/sqlc/db.go | Prepares/closes the pg GetAccountSecret statement. |
Files not reviewed (6)
- wallet/internal/sql/pg/sqlc/accounts.sql.go: Language not supported
- wallet/internal/sql/pg/sqlc/db.go: Language not supported
- wallet/internal/sql/pg/sqlc/querier.go: Language not supported
- wallet/internal/sql/sqlite/sqlc/accounts.sql.go: Language not supported
- wallet/internal/sql/sqlite/sqlc/db.go: Language not supported
- wallet/internal/sql/sqlite/sqlc/querier.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
Author
|
LGTM 🚀 ( |
Collaborator
Author
|
LGTM 🔐 ( |
yyforyongyu
force-pushed
the
impl-signer-account-secret
branch
2 times, most recently
from
59c3541 to
a1832b2
Compare
yyforyongyu
commented
Jun 4, 2026
yyforyongyu
left a comment
yyforyongyu
left a comment
Collaborator
Author
There was a problem hiding this comment.
LGTM (deepseek-v4-pro)
yyforyongyu
commented
Jun 4, 2026
yyforyongyu
left a comment
yyforyongyu
left a comment
Collaborator
Author
There was a problem hiding this comment.
LGTM (deepseek-v4-pro)
Collaborator
Author
|
LGTM 🔑 |
Collaborator
Author
|
LGTM 🐿️ ( |
yyforyongyu
force-pushed
the
impl-signer-account-secret
branch
from
a1832b2 to
9206c0c
Compare
Collaborator
Author
|
LGTM 🔑 |
yyforyongyu
commented
Jun 8, 2026
| // one account. The returned EncryptedPrivateKey is still ciphertext; | ||
| // callers must decrypt it through the wallet key vault before deriving | ||
| // private child keys. | ||
| GetAccountSecret(ctx context.Context, query GetAccountSecretQuery) ( |
Collaborator
Author
There was a problem hiding this comment.
i am questioning why do we need this method - when the wallet is unlocked, we will cache the secrets in the cache? thus that whenever we need to create secrets, we will use the cache to do so?
in addition what is the relationship among account secret, wallet secret, keyvault and cache?
Collaborator
Author
|
On the account-secret / cache / keyvault question: I think the durable encrypted account secret is still needed for SQL-only accounts, but the long-term domain boundary should be keyvault rather than signer or general wallet code fetching Current state in this branch:
Ideal shape:
So I agree this should not become the permanent public AccountStore-facing signing API. For this PR, either we keep |
Introduce GetAccountSecret on db.AccountStore so the wallet signer can read the encrypted account-level signing material that SQL backends persist for SQL-only accounts. The kvdb backend returns db.ErrAccountSecretUnavailable because its signing path still resolves through the legacy waddrmgr address manager. Add the matching pg/sqlite sqlc queries, account_secrets itest coverage for both SQL backends, and a GetAccountSecret method on the shared walletmock.Store so wallet-side tests can stub the new contract.
Add GetAccountSecret to runtimeCache so wallet-side consumers can read SQL-backed account signing material through a single boundary. The storeRuntimeCache implementation is a pass-through today, marked with the same TODO(yy) as the other cache reads so the cache layer can grow typed error wrapping later without churning callers.
When waddrmgr reports an account or scope miss, the legacy DeriveFromKeyPathCache fallback also fails for accounts that only live in the SQL store. Fall back to the encrypted account-level private key fetched from the store cache, decrypt it through keyVault, derive the requested branch and index, and zero intermediate key material before returning. Add SQLite-backed signing coverage for a SQL-only derived account in signer_test so the fallback exercises a real SQL store end-to-end and the existing kvdb-backed coverage keeps regressing the legacy path.
yyforyongyu
force-pushed
the
impl-signer-account-secret
branch
from
9206c0c to
b0f6271
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #1249, which auto-closed when its base branch impl-wallet-metadata-store was deleted; rebased onto sql-wallet. Adds the account-secret lookup contract, exposes store account secrets through the cache, and derives SQL-only account private keys via the keyVault in the signer.